Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#389] Add option 'blockSystemExit' to 'java' mojo #390

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Nov 9, 2023

This new option enables users to stop programs called by exec:java from calling System::exit, terminating not just the mojo but the whole Maven JVM.

Closes #389.
Relates to #388.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 9, 2023

@slawekjaranowski and other maintainers: Please review.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 9, 2023

I force-pushed a fix for the problem that only JDK 17+ needs the extra mavenOpts for Maven Invoker ITs. I thought, the system property would just be ignored, but older JDKs interpret the value "allow" for the security manager (SM) as a class name, trying to instantiate a SM of that name.

Please run the build again.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 9, 2023

@slawekjaranowski, another question: You seem to be re-starting some Windows ITs unrelated to my change. They also pass locally on my machine under Windows in the same JDK. Does that imply that you already know they are flaky? And do you know the reason why they are flaky? Is it because they run concurrently?

@slawekjaranowski
Copy link
Member

I will try to fix a ITs on windows ....

@slawekjaranowski
Copy link
Member

I hope fix ITs by #392 - please rebase

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 9, 2023

Rebased and pushed. There actually was a merge conflict, because we both added a new Maven profile in the same place. but no big deal.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 10, 2023

@slawekjaranowski, is there anything else stopping you from merging? I want to keep the difference between touch time and cycle time for this PR as low as possible, because frequent context switches are just bad for productivity.

On a side note, I am surprised that you need to explicitly approve not just the first build for a pull request but every single one. This kind of regulation is kind of unusual, most OSS projects only block the first one. It forces you to micro-manage the PR and wait for the build result while maybe you do not have much else to do with regard to reviewing it. So, probably you will also switch the context to something else first and then back again.

This new option enables users to stop programs called by 'exec:java'
from calling System::exit, terminating not just the mojo but the whole
Maven JVM.

Closes mojohaus#389.
Relates to mojohaus#388.
@slawekjaranowski slawekjaranowski merged commit 584c544 into mojohaus:master Nov 10, 2023
19 checks passed
@kriegaex kriegaex deleted the gh-389 branch November 11, 2023 10:37
@@ -279,6 +306,10 @@ public void run()
lookup.findStatic( bootClass, "main",
MethodType.methodType( void.class, String[].class ) );

if ( blockSystemExit )
{
System.setSecurityManager( new SystemExitManager( originalSecurityManager ) );
Copy link
Contributor

@TobiX TobiX Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this makes the mojo non-threadSafe and might introduce "funny" race-conditions. This should be at least documented.

(See also the similar feature in GMavenPlus: https://github.com/groovy/GMavenPlus/pull/146/files)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TobiX, interesting. Please elaborate. The link does not help much. What kind of race conditions do you envision? And what could be worse than the whole Maven process just shutting down hard when dealing with processes using System.exit()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to argue that a spurious System.exit() is a bad problem. If you cannot change the code running inside the maven process to avoid it, using a SecurityManager currently seems like the only solution. I'd just like to argue that it should be AT LEAST documented (or even changed in the mojo definition) that fiddling with the SecurityManager makes the mojo inherintly not thread-safe, which might lead to hard-to-debug problems when using this mojo in a threaded environment (M2E, mvnd or even -T) due to leaked SecurityMaangers (Due to race-conditions, an instance of SystemExitManager might linger after this mojo is finished)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all cases but System.exit being called, SystemExitManager passes through permission requests to the original anyway, so that would not be bad. The only effect of it lingering would be that no-one else could use System::exit anymore to shut down the JVM. But how would you imagine that even being even possible? The value is being reset in a finally block.

Please, like I said, give a concrete example. I am not against documenting possible problems or side effects, but there is no point in scaring users, leading them to maybe not use a useful feature, just because of something that will never happen. I am not a security manager expert. Actually, I am happy they will go away from the JVM. OTOH, I do not like System::exit either and find its existence harmful. There are better ways to terminate an application and its host JVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New option 'blockSystemExit' for 'java' mojo
3 participants